fix(vats): fix promise space reset() misbehavior #7710
Conversation
|
@dckc please run your init/upgrade-proposal test against this branch and see if it behaves better. I think the symptom of the buggy promise-space should have been that the @michaelfig please make sure this matches your original intentions for how @erights You mentioned that the Proxy could be safer, but it wasn't obvious to me what should be done.. let me know what should be changed. Concerns:
|
warner
force-pushed
the
7709-promise-space-reset
branch
from
1bd4116
to
80b8c26
Compare
destructure twice work-around: no joyBefore trying out this proposed fix, I thought this idea was worth a try:
The rewrite is:
I ran Proposals 1 and 2 failed due to unrelated errorsMy first smoke test attempt failed because I forgot to sufficiently fund the account that was supposed to upload the bundle. (The smoke test script seems to not handle this failure mode):
The second one failed due to a typo: I wrote
init, upgrade proposals succeed on 1st smoke test
upgraded behavior seems to work. note ps shows: Unfortunately, in an attempt to use upgrade affects wrong vat on 2nd smoke testI paused the smoke test script after
as expected, broken contract behavior observed: no Then I let the smoke test resume upgrading:
but note that the upgrade affected v46, the vat from the 1st smoke test run: ps shows |
|
I don't understand the "Chain deployment test" failure. It feels like the client somehow halted around https://github.com/Agoric/agoric-sdk/actions/runs/4956832069/jobs/8867776209?pr=7710#step:10:6894 (maybe while trying to provision the wallet?) , since no progress seems to be made after that point, but I don't see any errors other than the "peer connection lost", and I'm not even sure the client would have been treated as a peer. I need to compare this output against a successful run, and see where they diverge. |
it does! cherry-picking this fix into
repeating the experiment above, on the 2nd smoke test, we get the correct vat upgraded: and correct upgraded contract behavior: p.s. running the smoke test a 3rd time works too: chain log: smart wallet interaction with upgraded contract: |
| test('makePromiseSpace backed by store', async t => { | ||
| test('makePromiseSpace copied into store', async t => { |
There was a problem hiding this comment.
the new implementation is... slightly less puzzling to me than the old one :)
That is: this should wait for review by @michaelfig . But if he's not available, I think it should land; I'm willing to help maintain it.
warner
force-pushed
the
7709-promise-space-reset
branch
from
bf3f607
to
ab4bc10
Compare
warner
force-pushed
the
7709-promise-space-reset
branch
from
ab4bc10
to
876c16d
Compare
There was a problem hiding this comment.
On line 62-66
if (isPromise(valueP)) {
void valueP.then(value => save(name, value));
} else {
save(name, valueP);
}is NOT reentrancy safe, because isPromise does not check that valueP is a safe promise. IOW, it may still have an own malicious then method. Could check for a safe promise (`passStyleOf(valueP) === 'promise'), but better to just do
if (isPromise(valueP)) {
void E.when(valueP, value => save(name, value));
} else {
save(name, valueP);
}Even better in general would be
void E.when(valueP, value => save(name, value));unless there's a reason you need the synchronous shortcut. Do you?
Icing on the cake is that E.when knows how to do deep stack turn tracking.
I don't know. Given what I can glean from the code, callers (behavior functions) might Ok, so I should really move the @michaelfig I could really use your review on this, I want to make sure I didn't violate any of your other intentions like I did with
|
There was a problem hiding this comment.
Minor note. Feel free to ignore this round, but capturing while I see it
The code on 90-94 is correct, but I'd rewrite it as
const { log = noop, store = undefined, hooks = undefined } = opts;
const logHooks = store
? makeStoreHooks(store, log)
: hooks || makeLogHooks(log);
const { onAddKey, onSettled, onResolve, onReset } = logHooks;except that TS complains about it in a way that makes no sense to me. Certainly not worth untangling the TS problem today, so ignore for now.
warner
force-pushed
the
7709-promise-space-reset
branch
from
876c16d
to
9d57c66
Compare
There was a problem hiding this comment.
LGTM, thanks!
For the record, I do not thoroughly understand the code yet, so I'm really approving to remove my "request changes" block, enabling you to act on your existing approvals.
Everything I understood looks good, and I see no red flags. But this is certainly a weird abstraction that I'd like to come back to later to thoroughly understand. It looks important!
Previously, the reset() feature misbehaved in a common use pattern, where both the `reset` and `resolve` facets were extracted at the same time. The state was nominally held in `nameToState.get(name)`, but was sometimes used by closed-over `state` and `pk` variables. Which one you got depended upon when the Proxy `get` trap was called. If `resolve` was fetched early and retained across a `reset()` call, then `resolve()` would resolve the *old* promise. Later, when `consume` was used, the space would create a new Promise to satisfy the request, which would never be resolved. This changes the implementation to strictly keep/use the state in `nameToState`, and allow all resolve/reject/reset methods to work the same way no matter when they were retrieved (they close over `name` but not any state). closes #7709
per @erights: this prevents non-get operations from mutating the target and using it as a communications channel.
This is more defensive against sneaky promises, at the cost of delaying storage by a turn for non-promises.
Co-authored-by: Michael FIG <mfig@agoric.com>
warner
force-pushed
the
7709-promise-space-reset
branch
from
e35390a
to
e758c34
Compare
Previously, the reset() feature misbehaved in a common use pattern, where both the
resetandresolvefacets were extracted at the same time. The state was nominally held innameToState.get(name), but was sometimes used by closed-overstateandpkvariables. Which one you got depended upon when the Proxygettrap was called. Ifresolvewas fetched early and retained across areset()call, thenresolve()would resolve the old promise. Later, whenconsumewas used, the space would create a new Promise to satisfy the request, which would never be resolved.This changes the implementation to strictly keep/use the state in
nameToState, and allow all resolve/reject/reset methods to work the same way no matter when they were retrieved (they close overnamebut not any state).closes #7709